Skip to content

Add email verification for user registration#49

Merged
vieiralucas merged 25 commits intomainfrom
feat/email-verification-ci
Jan 25, 2026
Merged

Add email verification for user registration#49
vieiralucas merged 25 commits intomainfrom
feat/email-verification-ci

Conversation

@vieiralucas
Copy link
Copy Markdown
Member

@vieiralucas vieiralucas commented Jan 25, 2026

Summary

  • Implements Issue Feature: Implement email verification for new principals #46 - optional email verification during join flow
  • Add configurable email providers (SMTP or Resend API) to the server
  • Update CLI join command with --verification-code flag and interactive prompts
  • Update web app join page to handle verification step
  • Add comprehensive E2E tests for both CLI and web flows
  • Update CI workflows to run MailHog for email capture during tests

Test plan

  • Run cargo test --workspace --all-features - all tests pass
  • Run CLI E2E tests with MailHog: docker compose -f docker/docker-compose.test.yaml up -d && cargo test --package e2e-tests
  • Run web E2E tests: cd apps/zopp-web && npm run test:e2e
  • Verify CI passes (clippy, tests, e2e)
  • Manual test: start server with ZOPP_EMAIL_VERIFICATION_REQUIRED=true and verify join flow requires code

Closes #46


Summary by cubic

Adds optional email verification to the join flow across server, CLI, and web. When enabled, users must enter a 6-digit code sent by email. Implements #46.

  • New Features

    • Server: SMTP or Resend providers, verification code generation, pending verification storage, new VerifyEmail and ResendVerification RPCs; startup aborts if verification is required but misconfigured; invite consumption tracked; verification attempts limited; verification codes stored as Argon2id hashes.
    • Storage: adds user verified flag, invite consumed flag, and email_verifications table.
    • CLI: join supports interactive verification (with resend) and a --verification-code flag for non-interactive use.
    • Web: register page shows a verification step when required.
    • Tests/CI: MailHog added for captured emails; E2E tests for CLI and web; workflows updated to run MailHog and wait for readiness.
  • Migration

    • Run DB migrations (Postgres/SQLite) to add verification tables/columns.
    • Configure email provider:
      • SMTP: set ZOPP_EMAIL_PROVIDER=smtp and SMTP_* vars.
      • Resend: set ZOPP_EMAIL_PROVIDER=resend and RESEND_API_KEY.
      • Set ZOPP_EMAIL_FROM (and optional ZOPP_EMAIL_FROM_NAME).
    • Enable via ZOPP_EMAIL_VERIFICATION_REQUIRED=true (defaults to enabled when a provider is configured).
    • No change needed if email is not configured; join behaves as before.

Written for commit d9e309a. Summary will update on new commits.

Implements Issue #46 - optional email verification during join flow:

Server changes:
- Add configurable email provider (SMTP or Resend API)
- Add verification code generation and storage
- Add VerifyEmail and ResendVerificationEmail gRPC handlers
- Require valid verification before creating principal when enabled

CLI changes:
- Add --verification-code flag to join command
- Interactive flow: prompts for code when verification required

Web app changes:
- Update join page to handle verification step
- Show verification code input when server requires it

E2E testing:
- Add MailHog support via docker-compose.test.yaml
- Add email_verification.rs tests for CLI flow
- Add join-verification.spec.ts Playwright tests for web
- Update CI workflows to run MailHog for tests

Configuration via environment variables:
- ZOPP_EMAIL_VERIFICATION_REQUIRED (true/false)
- ZOPP_EMAIL_PROVIDER (smtp/resend)
- SMTP_HOST, SMTP_PORT, SMTP_USERNAME, SMTP_PASSWORD
- RESEND_API_KEY
- ZOPP_EMAIL_FROM, ZOPP_EMAIL_FROM_NAME
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

11 issues found across 67 files

Confidence score: 3/5

  • Email verification can be effectively bypassed if provider init fails in apps/zopp-server/src/main.rs, since required verification is silently disabled; that’s concrete user-impacting risk.
  • Invite consumption isn’t atomic in crates/zopp-store-postgres/src/lib.rs, so concurrent joins could reuse a token, and bootstrap invites in apps/zopp-server/src/handlers/auth.rs can be consumed by existing users when verification is required.
  • Migration in crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql backfills all users as unverified, which could lock out existing accounts if verification is intended only for new joins.
  • Pay close attention to apps/zopp-server/src/main.rs, apps/zopp-server/src/handlers/auth.rs, crates/zopp-store-postgres/src/lib.rs, crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql - verification/consume-flow correctness and migration impact.
Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="apps/zopp-server/src/handlers/verification.rs">

<violation number="1" location="apps/zopp-server/src/handlers/verification.rs:110">
P2: After incrementing failed attempts, the handler doesn’t immediately lock out when the counter reaches/exceeds MAX_ATTEMPTS. Under concurrent attempts this can return negative remaining and allow extra tries until a later request. Delete the verification and return the lockout response once `attempts >= MAX_ATTEMPTS`, and clamp remaining to non-negative.</violation>
</file>

<file name="apps/e2e-tests/tests/common/harness.rs">

<violation number="1" location="apps/e2e-tests/tests/common/harness.rs:630">
P2: Escape the email before interpolating it into the SQL command to avoid malformed queries or injection when test data contains single quotes.</violation>

<violation number="2" location="apps/e2e-tests/tests/common/harness.rs:659">
P2: Escape the email before interpolating it into the SQLite query to avoid malformed SQL or injection when test data contains single quotes.</violation>
</file>

<file name=".github/workflows/e2e.yaml">

<violation number="1" location=".github/workflows/e2e.yaml:35">
P2: Pin the MailHog image to a specific version (or digest) to keep CI runs reproducible; `latest` can change unexpectedly and break E2E tests.</violation>
</file>

<file name=".github/workflows/web-e2e.yaml">

<violation number="1" location=".github/workflows/web-e2e.yaml:67">
P2: Fail the workflow if MailHog does not become ready after the retry loop to avoid running tests without the email service.</violation>
</file>

<file name="apps/zopp-cli/src/commands/join.rs">

<violation number="1" location="apps/zopp-cli/src/commands/join.rs:348">
P2: The max-attempts guard never actually limits retries because it resets the counter, allowing unlimited verification attempts without a resend. Consider stopping the flow (or requiring a resend) once the limit is reached.</violation>
</file>

<file name="apps/zopp-web/tests/fixtures/verification-setup.ts">

<violation number="1" location="apps/zopp-web/tests/fixtures/verification-setup.ts:194">
P2: Unsafely interpolating `email` into a shell command/SQL query allows quote-breaking (and potential shell injection) in tests. Escape the email or use an argument-based execFileSync call to avoid the shell.</violation>
</file>

<file name="apps/zopp-server/src/handlers/auth.rs">

<violation number="1" location="apps/zopp-server/src/handlers/auth.rs:68">
P2: Bootstrap invites should still reject existing users when email verification is required. Add the same guard used in the non-verification flow so an AlreadyExists user can’t consume a bootstrap invite.</violation>
</file>

<file name="crates/zopp-store-postgres/src/lib.rs">

<violation number="1" location="crates/zopp-store-postgres/src/lib.rs:431">
P2: Consume invite should be atomic. Updating by token without `consumed = FALSE` lets concurrent requests both mark the invite used and proceed, so the invite can be consumed multiple times. Add a guard in the UPDATE to fail if already consumed and use rows_affected() to detect it.</violation>
</file>

<file name="apps/zopp-server/src/main.rs">

<violation number="1" location="apps/zopp-server/src/main.rs:319">
P1: If email verification is configured as required, failing to initialize the provider should abort startup instead of silently disabling verification. Otherwise a misconfigured server can accept joins without verification.</violation>
</file>

<file name="crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql">

<violation number="1" location="crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql:6">
P2: This backfills all existing users as unverified. If verification is only intended for new join flows, existing accounts will suddenly be marked unverified. Consider updating existing rows to true during the migration to preserve current user state.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/zopp-server/src/main.rs
Comment thread apps/zopp-server/src/handlers/verification.rs Outdated
Comment thread apps/e2e-tests/tests/common/harness.rs Outdated
Comment thread apps/e2e-tests/tests/common/harness.rs Outdated
Comment thread .github/workflows/e2e.yaml Outdated
Comment thread apps/zopp-cli/src/commands/join.rs Outdated
Comment thread apps/zopp-web/tests/fixtures/verification-setup.ts Outdated
Comment thread apps/zopp-server/src/handlers/auth.rs
Comment thread crates/zopp-store-postgres/src/lib.rs Outdated
Comment thread crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql Outdated
Fixes issues identified by Cubic AI reviewer:

P1 (Critical):
- Server now aborts startup if email verification is required but
  provider initialization fails, preventing silent verification bypass

P2 (Security/Correctness):
- Verification handler now locks out and deletes record immediately when
  MAX_ATTEMPTS is reached, preventing negative remaining attempts
- SQL injection prevention: escape single quotes in email before SQL
  interpolation in test harness and TypeScript fixtures
- Pin MailHog image to v1.0.1 for reproducible CI builds
- Fail web-e2e workflow if MailHog doesn't become ready
- CLI join flow now requires resend when server returns 0 attempts
  remaining, preventing unlimited local retries
- Bootstrap invites now reject verified existing users when email
  verification is required, matching non-verification flow behavior
- consume_invite is now atomic in both PostgreSQL and SQLite stores,
  checking consumed=FALSE in UPDATE to prevent concurrent consumption
- Migration now sets existing users as verified (DEFAULT TRUE) then
  changes default to FALSE for new users, preserving existing accounts

Also updates sqlx metadata for both stores.
The test was creating a user WITH a principal, but according to our
implementation:
- Users with principals are verified immediately (no email verification)
- Users without principals are unverified (email verification required)

Changed test to create user without principal to properly test the
mark_user_verified flow used after email verification.
Removed mock_smtp module and mailin-embedded dependency since we use
MailHog for email testing. mailin-embedded was pulling in aws-lc-rs
via rustls which conflicted with kube's ring-based rustls configuration,
causing K8s tests to fail.
- Add getVerificationCodeFromMailHog helper function
- Update setupTestData to handle email verification when MAILHOG_API_URL is set
- Use --verification-code flag for CLI join command
- Increase maxBuffer to prevent ENOBUFS errors
- Also increase operator sync timeout for debugging CI
Use getByRole('heading') instead of getByText(/verification code/i)
which was matching multiple elements. Also use exact placeholder match.
@vieiralucas
Copy link
Copy Markdown
Member Author

@cubic-dev-ai All issues from your review have been addressed in commit 0cd5a81. Please re-review.

@cubic-dev-ai
Copy link
Copy Markdown

cubic-dev-ai Bot commented Jan 25, 2026

@cubic-dev-ai All issues from your review have been addressed in commit 0cd5a81. Please re-review.

@vieiralucas I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 issues found across 75 files

Confidence score: 3/5

  • crates/zopp-store-postgres/src/lib.rs has a concrete bug risk: consume_invite can consume revoked tokens since the update doesn’t filter revoked = FALSE, which can let revoked invites be treated as valid.
  • UI behavior risk in apps/zopp-web/src/pages/register.rs: if verification succeeds but complete_registration_impl fails, the UI remains stuck in a loading state because verifying isn’t reset and errors route to the join form.
  • Given the medium-severity functional issues, there’s some user-impacting regression risk, so this feels slightly risky to merge without fixes.
  • Pay close attention to crates/zopp-store-postgres/src/lib.rs, apps/zopp-web/src/pages/register.rs, apps/zopp-web/tests/fixtures/test-setup.ts, crates/zopp-store-sqlite/src/lib.rs - functional correctness and test reliability.
Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="crates/zopp-store-postgres/src/lib.rs">

<violation number="1" location="crates/zopp-store-postgres/src/lib.rs:441">
P2: consume_invite can mark revoked invites as consumed because the update doesn’t exclude revoked tokens. Revoked invites are treated as not found elsewhere, so this should also filter revoked = FALSE (and return NotFound for revoked tokens).</violation>
</file>

<file name="apps/zopp-web/tests/fixtures/test-setup.ts">

<violation number="1" location="apps/zopp-web/tests/fixtures/test-setup.ts:34">
P2: Add a fetch timeout/abort so the retry loop can honor `timeoutMs` instead of hanging on a stalled request.</violation>
</file>

<file name="apps/zopp-web/src/pages/register.rs">

<violation number="1" location="apps/zopp-web/src/pages/register.rs:79">
P2: If `complete_registration_impl` fails after verification succeeds, the verification UI stays in a loading state with no error because `verifying` isn’t reset and errors go to the join form. Add an `else` branch to clear `verifying` and show a verification error.</violation>
</file>

<file name="crates/zopp-store-sqlite/src/lib.rs">

<violation number="1" location="crates/zopp-store-sqlite/src/lib.rs:4331">
P3: The new tests use `matches!` without asserting the result, so they don't actually verify the error type and will pass even if the wrong error is returned. Use `assert!(matches!(...))` (or `assert!(err == ...)`) to make the tests effective.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread crates/zopp-store-postgres/src/lib.rs Outdated
Comment thread apps/zopp-web/tests/fixtures/test-setup.ts Outdated
Comment thread apps/zopp-web/src/pages/register.rs
Comment thread crates/zopp-store-sqlite/src/lib.rs Outdated
Fixes 4 issues identified by Cubic AI reviewer:

P2 (Security/Correctness):
- consume_invite now filters revoked=FALSE to prevent consuming revoked
  invites in both PostgreSQL and SQLite stores
- Add fetch timeout/abort to test-setup.ts getVerificationCodeFromMailHog
  to honor overall timeout instead of hanging on stalled requests
- Fix verification UI to use correct signals when complete_registration_impl
  fails after successful verification, preventing stuck loading state

P3 (Test quality):
- Add assert!() around matches!() calls in SQLite tests to actually
  verify error types instead of silently passing

Also updates sqlx metadata for both stores.
@vieiralucas
Copy link
Copy Markdown
Member Author

@cubic-dev-ai All 4 issues from your re-review have been addressed in commit d7832a4. Please re-review.

@cubic-dev-ai
Copy link
Copy Markdown

cubic-dev-ai Bot commented Jan 25, 2026

@cubic-dev-ai All 4 issues from your re-review have been addressed in commit d7832a4. Please re-review.

@vieiralucas I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 75 files

Confidence score: 4/5

  • This PR looks safe to merge overall; the noted issues are low severity and limited to test/comment accuracy rather than core runtime behavior.
  • Potential flakiness in apps/e2e-tests/tests/email_verification.rs if the generated code can match "000000", which could intermittently fail the test suite.
  • Pay close attention to apps/e2e-tests/tests/email_verification.rs and apps/e2e-tests/tests/common/harness.rs - test flakiness risk and misleading fallback comment.
Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="apps/e2e-tests/tests/common/harness.rs">

<violation number="1" location="apps/e2e-tests/tests/common/harness.rs:235">
P3: The comment claims a fallback to a mock SMTP server, but the implementation errors when MailHog isn’t available. Update the comment to match the behavior (or implement the fallback) to avoid misleading test setup guidance.</violation>
</file>

<file name="apps/e2e-tests/tests/email_verification.rs">

<violation number="1" location="apps/e2e-tests/tests/email_verification.rs:77">
P3: Using "000000" as the invalid verification code can occasionally match a real generated code (range includes 000000), making this test flaky. Use a non-numeric placeholder or derive an invalid code from the real one to guarantee failure.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/e2e-tests/tests/common/harness.rs Outdated
Comment thread apps/e2e-tests/tests/email_verification.rs Outdated
P3 fixes:
- Update harness.rs comment to accurately state MailHog is required
  (no fallback to mock SMTP exists)
- Use non-numeric "XXXXXX" as invalid verification code in test to
  guarantee it never matches a real generated code
@vieiralucas
Copy link
Copy Markdown
Member Author

@cubic-dev-ai Fixed the 2 P3 issues in commit 2f4251f. Please re-review.

@cubic-dev-ai
Copy link
Copy Markdown

cubic-dev-ai Bot commented Jan 25, 2026

@cubic-dev-ai Fixed the 2 P3 issues in commit 2f4251f. Please re-review.

@vieiralucas I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 issues found across 75 files

Confidence score: 3/5

  • Resend flow in apps/zopp-server/src/handlers/verification.rs can invalidate a valid code before confirming email delivery, which risks blocking real users if sending fails.
  • Several test-harness issues can lead to flakiness or false positives (readiness check, MailHog cleanup, and fixed invalid code), so there is some risk of unreliable CI signal.
  • These are moderate-severity and user-impacting in one case, so there is some merge risk but likely manageable with quick follow-ups.
  • Pay close attention to apps/zopp-server/src/handlers/verification.rs, apps/zopp-web/tests/fixtures/verification-setup.ts, apps/e2e-tests/tests/common/harness.rs, apps/e2e-tests/tests/email_verification.rs - verification resend behavior and test flakiness/cleanup correctness.
Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="apps/zopp-web/tests/fixtures/verification-setup.ts">

<violation number="1" location="apps/zopp-web/tests/fixtures/verification-setup.ts:43">
P2: waitForServer should only return true when the readiness endpoint returns success; otherwise tests can proceed while the server is still starting.</violation>
</file>

<file name="apps/zopp-server/src/handlers/verification.rs">

<violation number="1" location="apps/zopp-server/src/handlers/verification.rs:324">
P2: Resend invalidates the existing verification code before confirming the email was sent. If the send fails, the previous code is lost and the user cannot verify with the code they already received.</violation>
</file>

<file name="apps/e2e-tests/tests/email_verification.rs">

<violation number="1" location="apps/e2e-tests/tests/email_verification.rs:139">
P3: Using "000000" as the invalid verification code can intermittently pass because the generator can legitimately produce 000000. This makes the E2E test flaky.</violation>
</file>

<file name="apps/e2e-tests/tests/common/harness.rs">

<violation number="1" location="apps/e2e-tests/tests/common/harness.rs:269">
P2: Don’t ignore MailHog clear failures; stale messages can cause tests to pick up an old verification code and pass/fail nondeterministically. Propagate the error so the test fails fast when cleanup fails.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/zopp-web/tests/fixtures/verification-setup.ts Outdated
Comment thread apps/zopp-server/src/handlers/verification.rs Outdated
Comment thread apps/e2e-tests/tests/common/harness.rs Outdated
Comment thread apps/e2e-tests/tests/email_verification.rs Outdated
P2 fixes:
- verification-setup.ts: Check response.ok before returning true from
  waitForServer to ensure server is actually ready
- verification.rs: Send email BEFORE updating DB in resend flow so old
  code is preserved if email sending fails
- harness.rs: Propagate MailHog clear errors instead of ignoring them
  to fail fast on cleanup issues that could cause flaky tests

P3 fixes:
- email_verification.rs: Fix remaining "000000" to use "XXXXXX"
@vieiralucas
Copy link
Copy Markdown
Member Author

@cubic-dev-ai Please do a full re-review of the PR.

@cubic-dev-ai
Copy link
Copy Markdown

cubic-dev-ai Bot commented Jan 25, 2026

@cubic-dev-ai Please do a full re-review of the PR.

@vieiralucas I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 75 files

Confidence score: 3/5

  • Potential user-impacting bug in apps/zopp-server/src/handlers/auth.rs: creating the verification record before sending email can prevent resend attempts if the email fails, so some users may never receive verification.
  • There’s a security hardening gap in crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql where verification codes are stored in plain text; hashing would reduce risk in a DB compromise scenario.
  • Pay close attention to apps/zopp-server/src/handlers/auth.rs, crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql, apps/zopp-web/tests/fixtures/verification-setup.ts - verification email flow ordering, plaintext codes, and test gRPC URL misconfiguration.
Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="apps/zopp-web/tests/fixtures/verification-setup.ts">

<violation number="1" location="apps/zopp-web/tests/fixtures/verification-setup.ts:151">
P2: grpcWebUrl points to an unused Envoy port even though Envoy is not started, which will break any test using verificationContext.grpcWebUrl. Use the server port when testing gRPC directly or start Envoy.</violation>
</file>

<file name="apps/zopp-server/src/handlers/auth.rs">

<violation number="1" location="apps/zopp-server/src/handlers/auth.rs:118">
P2: Verification record is stored before email is sent. If email fails, retries won't resend the email because a valid verification record exists. Send email first (like `resend_verification` does) to ensure user receives code before record is committed.</violation>
</file>

<file name="crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql">

<violation number="1" location="crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql:19">
P2: Consider hashing verification codes rather than storing them in plain text. Even for short-lived codes with attempt limiting, hashing provides defense-in-depth against database compromise scenarios (leaked backups, SQL injection, insider threats). Use a fast hash like SHA-256 for the code since it's already low-entropy and rate-limited—or Argon2id for stronger protection.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread apps/zopp-web/tests/fixtures/verification-setup.ts Outdated
Comment thread apps/zopp-server/src/handlers/auth.rs Outdated
Comment thread crates/zopp-store-postgres/migrations/20260125000001_add_email_verification.sql Outdated
P2 fixes from Cubic full re-review:
- auth.rs: Send verification email BEFORE storing record, matching
  the resend flow fix. Prevents storing record when email fails.
- verification-setup.ts: Use server port for grpcWebUrl since Envoy
  isn't started in this test setup.

Also update CLAUDE.md with clearer Cubic workflow instructions.
- Add generic argon2_hash and argon2_hash_raw functions to zopp-crypto
- Refactor CLI to use zopp-crypto for key derivation and verification hash
- Change storage schema from 'code' to 'code_hash' in both SQLite and Postgres
- Update server handlers to hash codes before storage and comparison
- Fix MailHog client to use v1 API for delete (v2 doesn't support DELETE)
- Add create_unverified_test_user helper for testing unverified principal flows
- Update E2E tests to use has_verification_record since plaintext codes are no longer stored
@vieiralucas
Copy link
Copy Markdown
Member Author

@cubic-dev-ai Please do a full re-review of the PR.

@cubic-dev-ai
Copy link
Copy Markdown

cubic-dev-ai Bot commented Jan 25, 2026

@cubic-dev-ai Please do a full re-review of the PR.

@vieiralucas I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 79 files

Confidence score: 3/5

  • Storing verification codes as plain SHA-256 in crates/zopp-storage/src/lib.rs enables offline brute-force if the table leaks; a slow hash or HMAC is needed to avoid a concrete security risk.
  • apps/zopp-server/src/handlers/auth.rs appears to swallow storage lookup errors; non-NotFound failures could still send emails and mask internal errors, which is user-impacting.
  • Score reflects a notable security concern plus error-handling uncertainty; overall change seems manageable once those are addressed.
  • Pay close attention to crates/zopp-storage/src/lib.rs, apps/zopp-server/src/handlers/auth.rs, crates/zopp-store-sqlite/migrations/20260125000001_add_email_verification.sql - hashing approach and error handling consistency.

Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="crates/zopp-storage/src/lib.rs">

<violation number="1" location="crates/zopp-storage/src/lib.rs:424">
P1: Storing verification codes as plain SHA-256 hashes makes offline brute-force trivial if the verification table is leaked. Use a slow password hash (e.g., Argon2id with per-record salt) or a server-secret HMAC so short codes aren’t crackable offline.</violation>
</file>

<file name="crates/zopp-store-sqlite/migrations/20260125000001_add_email_verification.sql">

<violation number="1" location="crates/zopp-store-sqlite/migrations/20260125000001_add_email_verification.sql:18">
P2: Comment says 'SHA-256 hash' but PR description states 'Argon2id hashes'. If the implementation uses Argon2id (as it should for brute-force resistance), update the comment to accurately reflect the hashing algorithm.</violation>
</file>

<file name="apps/zopp-server/src/handlers/auth.rs">

<violation number="1" location="apps/zopp-server/src/handlers/auth.rs:101">
P2: Don’t swallow storage errors when checking for an existing verification. If the lookup fails for reasons other than NotFound, the join should return an internal error instead of sending an email and potentially leaving no verification record.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread crates/zopp-storage/src/lib.rs Outdated
Comment thread crates/zopp-store-sqlite/migrations/20260125000001_add_email_verification.sql Outdated
Comment thread apps/zopp-server/src/handlers/auth.rs Outdated
- Update comments from 'SHA-256 hash' to 'Argon2id hash' in storage structs
- Update migration comments to reflect Argon2id usage
- Fix auth.rs to properly handle storage errors vs NotFound when checking
  for existing verification records
@vieiralucas
Copy link
Copy Markdown
Member Author

@cubic-dev-ai Please do a full re-review of the PR.

@cubic-dev-ai
Copy link
Copy Markdown

cubic-dev-ai Bot commented Jan 25, 2026

@cubic-dev-ai Please do a full re-review of the PR.

@vieiralucas I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 79 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.

@vieiralucas vieiralucas merged commit 110e686 into main Jan 25, 2026
21 checks passed
@vieiralucas vieiralucas deleted the feat/email-verification-ci branch January 25, 2026 23:31
vieiralucas added a commit that referenced this pull request Mar 2, 2026
Remove all SaaS/cloud features that were added in PRs #49-#63 to refocus
the project as an OSS self-hostable secrets manager. Preserves useful
self-hosted infrastructure (email verification, Prometheus metrics, Helm
charts, Amber Terminal web UI).

Removed:
- Organization and billing gRPC RPCs, messages, and proto definitions
- Organization/billing server handlers and backend delegation
- CLI `org` subcommand and organization command module
- Store trait organization methods + SQLite/PostgreSQL/NoopStore implementations
- Organization types (OrganizationId, OrganizationRole, Plan, SubscriptionStatus, etc.)
- zopp-billing crate (Stripe integration)
- Terraform AWS infrastructure (infra/terraform/)
- Drop migrations added for both backends to clean up existing databases
- Stale .sqlx query metadata for removed org/billing queries regenerated
vieiralucas added a commit that referenced this pull request Mar 2, 2026
…73)

* refactor: strip organizations, billing, and Terraform cloud features

Remove all SaaS/cloud features that were added in PRs #49-#63 to refocus
the project as an OSS self-hostable secrets manager. Preserves useful
self-hosted infrastructure (email verification, Prometheus metrics, Helm
charts, Amber Terminal web UI).

Removed:
- Organization and billing gRPC RPCs, messages, and proto definitions
- Organization/billing server handlers and backend delegation
- CLI `org` subcommand and organization command module
- Store trait organization methods + SQLite/PostgreSQL/NoopStore implementations
- Organization types (OrganizationId, OrganizationRole, Plan, SubscriptionStatus, etc.)
- zopp-billing crate (Stripe integration)
- Terraform AWS infrastructure (infra/terraform/)
- Drop migrations added for both backends to clean up existing databases
- Stale .sqlx query metadata for removed org/billing queries regenerated

* fix(deps): update bytes 1.10.1 → 1.11.1 (RUSTSEC-2026-0007)

Fixes integer overflow vulnerability in BytesMut::reserve that could
cause out-of-bounds memory access in release builds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: Implement email verification for new principals

1 participant